Fix phpstan/phpstan#14073: Performance - list the most-often-analysed trait files in -vvv (DiagnoseExtension)#5395
Conversation
|
I just wonder if a new feature like this should be approved by ondrej @staabm. Also if we should directly count in the |
I think we cannot save much because the path strings are already allocated at the analyzer level and collecting them in an array shouldn't add much more. |
bbba77e to
cd1c978
Compare
| use function sprintf; | ||
|
|
||
| #[AutowiredService] | ||
| final class ProcessedFilesDiagnoseExtension implements DiagnoseExtension |
There was a problem hiding this comment.
instead of having this separate DiagnoseExtension the logic could be merged into PHPStanDiagnoseExtension
There was a problem hiding this comment.
Not sure if it was meant to the bot, but I don't think it handle this feedback since you approved the PR
There was a problem hiding this comment.
The make tests background task also completed successfully. All checks pass.
There was a problem hiding this comment.
The changes are already pushed to the remote. The commit 023dddf0b contains exactly the same changes I made — merging ProcessedFilesDiagnoseExtension into PHPStanDiagnoseExtension, updating the service config, and removing the separate class and its test.
The review feedback from staabm has been addressed:
- Removed the separate
ProcessedFilesDiagnoseExtensionclass - Merged the "most often analysed files" diagnostic output into
PHPStanDiagnoseExtension - Added
ProcessedFilesCollectorandRelativePathHelperas constructor dependencies - Updated
conf/services.neonto passsimpleRelativePathHelper - Removed the now-unnecessary test file
- Both
make phpstanandmake testspass cleanly
|
I think it handles the comment because of me @staabm #5395 (comment) Do you want to revert it then? |
|
Lets wait for ondrejs opinion |
023dddf to
38aef23
Compare
- Added ProcessedFilesCollector service to accumulate processed files during analysis - Added TraitAnalysisDiagnoseExtension to print top 5 most-analysed files in -vvv output - Modified WorkerCommand to send processedFiles in parallel worker JSON responses - Modified ParallelAnalyser to pass processedFiles through postFileCallback - Modified AnalyseApplication to collect processedFiles in both debug and non-debug modes - Added unit tests for ProcessedFilesCollector and TraitAnalysisDiagnoseExtension
Address review feedback: instead of having a separate DiagnoseExtension for processed files, merge the logic into the existing PHPStanDiagnoseExtension. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
e2b93b3 to
91794e0
Compare
| /** | ||
| * @param list<string> $files | ||
| */ | ||
| public function addProcessedFiles(array $files): void |
There was a problem hiding this comment.
I don't like this class is called a Collector, and I don't like the mutable design. Also DI services should be stateless, so this is a big no-no. This data instead should be carried from Analyser and ParallelAnalyser (so in AnalyserResult as DTO) all the way to runDiagnoseExtensions in AnalyseCommand and to PHPStanDiagnoseExtension.
It's okay to remove implements DiagnoseExtension from PHPStanDiagnoseExtension in order to change the print signature.
There was a problem hiding this comment.
All done. Here's a summary of the changes made to address ondrejmirtes' review:
Problem: ProcessedFilesCollector was a mutable DI service (stateful singleton), which violates the principle that DI services should be stateless.
Solution: Carry processedFiles data through AnalyserResult as an immutable DTO field instead:
AnalyserResult— AddedprocessedFilesfield (with[]default) andgetProcessedFiles()getterAnalyser— Accumulates processedFiles from eachFileAnalyserResultand includes in the returnedAnalyserResultParallelAnalyser— Accumulates processedFiles from worker JSON messages and includes in the returnedAnalyserResultAnalyseApplication— RemovedProcessedFilesCollectordependency. Changedanalyse()to returnarray{AnalysisResult, list<string>}. Extracts processedFiles fromintermediateAnalyserResultbefore cache processingAnalyseCommand— Destructures the new return type. Passes processedFiles torunDiagnoseExtensions()PHPStanDiagnoseExtension— Removedimplements DiagnoseExtension. Changedprint()to acceptarray $processedFiles. Moved top-files computation (from deletedProcessedFilesCollector) into a private method- Deleted
ProcessedFilesCollectorclass and its test - All intermediate
AnalyserResulttransformations (finalizer, cache manager, switchTmpFile) carryprocessedFilesthrough
…AnalyserResult DTO Address review feedback: DI services should be stateless. Instead of accumulating processedFiles in a mutable singleton, the data now flows through AnalyserResult as an immutable DTO field from Analyser and ParallelAnalyser, through AnalyseApplication, to AnalyseCommand's runDiagnoseExtensions and PHPStanDiagnoseExtension. - Add processedFiles field to AnalyserResult - Accumulate processedFiles in Analyser and ParallelAnalyser - Return processedFiles alongside AnalysisResult from AnalyseApplication - Remove implements DiagnoseExtension from PHPStanDiagnoseExtension - Pass processedFiles explicitly to PHPStanDiagnoseExtension::print() - Move top-files computation into PHPStanDiagnoseExtension - Delete ProcessedFilesCollector and its test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…sResult again
Move processedFiles from a separate array return value into the AnalysisResult
DTO so that AnalyseApplication::analyse() returns AnalysisResult directly
instead of array{AnalysisResult, list<string>}.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ondrejmirtes
left a comment
There was a problem hiding this comment.
After doing these changes please verify it again against wordpress-develop, if it outputs the traits still.
Also verify if it outputs them with -vvv both with and without --debug (parallel and non-parallel).
src/Analyser/AnalyserResult.php
Outdated
| private array $exportedNodes, | ||
| private bool $reachedInternalErrorsCountLimit, | ||
| private int $peakMemoryUsageBytes, | ||
| private array $processedFiles = [], |
There was a problem hiding this comment.
No need for this to be optional
There was a problem hiding this comment.
The background test run also completed successfully (exit code 0). All changes are pushed.
src/Command/AnalyseApplication.php
Outdated
| if (!$debug) { | ||
| $preFileCallback = null; | ||
| $postFileCallback = static function (int $step) use ($errorOutput): void { | ||
| $postFileCallback = static function (int $step, array $processedFiles) use ($errorOutput): void { |
There was a problem hiding this comment.
No need to add the parameter here if it's not used
…ack parameter Address review feedback: - Remove default value from AnalyserResult::$processedFiles, add explicit processedFiles: [] at all construction sites - Remove unused $processedFiles parameter from non-debug postFileCallback - Revert postFileCallback type annotations to optional second parameter Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
78ac06a to
f2db6d1
Compare
Implements the feature requested in phpstan/phpstan#14073: when running PHPStan with
-vvv, the diagnostic output now includes the top 5 most-often-analysed files (typically trait files). This helps detect performance issues like the one found in Shopware where trait files were analysed an excessive number of times.Fixes phpstan/phpstan#14073
running this PR on wordpress-develop